fix(@angular/ssr): remove CSR fallback for invalid hosts#32579
fix(@angular/ssr): remove CSR fallback for invalid hosts#32579alan-agius4 merged 1 commit intoangular:mainfrom
Conversation
alan-agius4
added
action: review
angular-robot
bot
added
detected: breaking change
Previously, when a request contained an unrecognized host header, the server would fallback to serving the client-side application (CSR) as a temporary migration path. This commit removes this fallback behavior. Requests with invalid or unrecognized host headers will now strictly return a 400 Bad Request response. BREAKING CHANGE: The server no longer falls back to Client-Side Rendering (CSR) when a request fails host validation. Requests with unrecognized 'Host' headers will now return a 400 Bad Request status code. Users must ensure all valid hosts are correctly configured in the 'allowedHosts' option.
alan-agius4
force-pushed
the
ssr-csr-fallback
branch
from
aa862b8 to
55401ca
Compare
dgp1130
left a comment
dgp1130
left a comment
There was a problem hiding this comment.
LGTM for this change, but at a higher level, I'm a bit worried this could present very breaking behavior for most apps.
IIUC: If a developer doesn't do anything (just ng update), and blindly updates to the next major, they won't get any CI failures or local issues, but upon deploying they will fail all requests. That could catch a lot devs off guard. Is that an accurate assessment of the new behavior?
It's tricky to find a balance which makes this a visible breakage for existing apps while also support new developers creating a new app and not wanting to think about hostnames yet.
Two alternatives I can think of:
- Validate
allowedHostsat startup (maybe limited to production builds?), so the server fails to even start without it. That would hopefully trigger a CI error in most environments and be visible locally. - Add some kind of
allowEmptyHostsInDevoption and enable it automatically inng new, but then cause a type error forallowedHosts: undefined | [], allowEmptyHostsInDev: false | undefined. This way upgrading apps would get a compile-error that a host needs to be specified while not blocking new apps from dev. Then once we're sure all existing apps have a host listed, we can dropallowEmptyHostsInDevin the next major.
Do either of those options make sense? Is there a path to making this new constraint up front and visible to migrating apps?
| console.error( | ||
| `ERROR: ${(error as Error).message}` + | ||
| 'Please provide a list of allowed hosts in the "allowedHosts" option in the "CommonEngine" constructor.', |
There was a problem hiding this comment.
Consider: Do we still want a separate log statement since we have an unconditionalthrow below? I feel like this would result in double-logging this error under more circumstances.
There was a problem hiding this comment.
In this case the throw will be passed to the express error which will not print the message in the console.
This isn't feasible because
Again a build error is not feasible here due to the same thing as described above, in dev-server, we pass the Vite's |
alan-agius4
added
action: merge
|
This PR was merged into the repository. The changes were merged into the following branches:
|
2 participants
Previously, when a request contained an unrecognized host header, the server would fallback to serving the client-side application (CSR) as a temporary migration path.
This commit removes this fallback behavior. Requests with invalid or unrecognized host headers will now strictly return a 400 Bad Request response.
BREAKING CHANGE: The server no longer falls back to Client-Side Rendering (CSR) when a request fails host validation. Requests with unrecognized headers will now return a 400 Bad Request status code. Users must ensure all valid hosts are correctly configured in the option.